[Stdlib] Fix entry-point-based process args by CodaFi · Pull Request #3108 · swiftlang/swift

CodaFi · 2016-06-21T19:08:34Z

What's in this pull request?

Provides a new fallback for Process arguments for those instances where we do not own main (e.g. Frameworks, Objective-C owns main.m or main.c, etc.). This includes a number of platform-specific specializations of a new runtime function _swift_get_args and a new thread-safe interface to Process.unsafeArgv.

Resolved bug number: (SR-1119)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

jrose-apple · 2016-06-21T20:39:01Z

stdlib/public/stubs/Stubs.cpp

I think you need to reset arg and size here.

Both of those are purely out parameters and I can't find any precedence for resetting them.

From the man page:

Alternatively, before calling getline(), *lineptr can contain a pointer to a malloc(3)-allocated buffer *n bytes in size.

That is, subsequent calls would overwrite what the first call allocated.

gribozavr · 2016-06-21T22:11:54Z

stdlib/public/stubs/Stubs.cpp

I think using /proc/self/cmdline in the error will help a lot of people who run Swift in restricted environments. Seeing a message about /proc makes it immediately obvious what's wrong (/proc not available, or not mounted etc.)

gribozavr · 2016-06-21T22:13:45Z

stdlib/public/core/Process.swift

Expected should be nil. We are initializing _swift_stdlib_ProcessUnsafeArgv from nil to newArgs.

Yep. Accidentally committed while in the middle of refactoring.

I feel like we should be able to get rid of _Box and the ARC mess altogether, by doing something like this:

static var unsafeArgv = _unsafeArgvFromMain ?? _unsafeArgvFromRuntime static var _unsafeArgvFromRuntime = _swift_stdlib_getUnsafeArgcArgv(&_argc)

I guess that's not 100% thread-safe because _unsafeArgvFromMain could be set from main while being read on another thread spawned prior to entering main, but something like this could work, right?

There's not ARC or boxes now, everything is just pointers.

jrose-apple · 2016-06-23T23:00:02Z

I think we should also stop using main to set argc/argv except in interpreter mode, but we can do that in a follow-up commit.

gribozavr · 2016-06-24T01:10:08Z

stdlib/public/core/Process.swift

The doc comment should be above the attribute.

CodaFi · 2016-07-11T20:08:20Z

I spoke with @trentxintong earlier today and he does not believe that the old static initializer problems will manifest themselves here. I've folded everything into static lets.

CodaFi · 2016-07-11T23:31:13Z

@swift-ci please test.

jrose-apple · 2016-07-12T00:19:44Z

lib/Immediate/ImmediateImpl.h

Nitpick: please make this a proper doc comment, so that it shows up in Quick Help.

I'll get it when I remove the compiler intrinsic.

jrose-apple · 2016-07-12T00:26:08Z

Happy to be getting this nailed down!

CodaFi · 2016-07-12T07:42:28Z

@swift-ci please test.

CodaFi · 2016-07-12T08:38:34Z

The failures look unrelated, but I want a clean merge.

Provides a new fallback for Process arguments for those instances where we do not own main (e.g. Frameworks, Objective-C owns main.m or main.c, etc.). This includes a number of platform-specific specializations of argument grabbing logic and a new thread-safe interface to Process.unsafeArgv. main() | _NSGetArgc/_NSGetArgv | /proc/self/cmdline | __argc/__argv --------|--------------------------|------------------------|--------------- Scripts | OS X, iOS, tvOS, watchOS | Linux, FreeBSD, Cygwin | Windows For interpreted Swift where we must filter out the arguments we now do so by loading the standard library and calling into new SPI to override the arguments that would have been grabbed by the runtime. This implementation completely subsumes the use of the entry point '_stdlib_didEnterMain' and it will be removed in a future commit.

CodaFi · 2016-07-14T08:10:53Z

@swift-ci please test and merge.

CodaFi · 2016-07-14T08:53:40Z

Finally! The build failures aren't my fault.

CodaFi changed the title ~~Fix entry-point-based process args~~ [Stdlib] Fix entry-point-based process args Jun 21, 2016

CodaFi force-pushed the argumental branch from 31b69c0 to b43b462 Compare June 21, 2016 19:13

jrose-apple reviewed Jun 21, 2016
View reviewed changes

CodaFi force-pushed the argumental branch 2 times, most recently from a2d3ca1 to b81002b Compare June 21, 2016 21:55

gribozavr reviewed Jun 21, 2016
View reviewed changes

CodaFi force-pushed the argumental branch from b81002b to 0f1b64b Compare June 21, 2016 22:12

gribozavr reviewed Jun 21, 2016
View reviewed changes

CodaFi force-pushed the argumental branch 3 times, most recently from d5e60bd to c0cbd40 Compare June 24, 2016 00:16

gribozavr reviewed Jun 24, 2016
View reviewed changes

stdlib/public/core/Process.swift Outdated

Copy link

Contributor

gribozavr Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc comment should be above the attribute.

CodaFi force-pushed the argumental branch 3 times, most recently from f0ad629 to 4314cd0 Compare July 11, 2016 23:30

jrose-apple reviewed Jul 12, 2016
View reviewed changes

CodaFi force-pushed the argumental branch from 4314cd0 to a169c1b Compare July 12, 2016 07:35

CodaFi force-pushed the argumental branch 8 times, most recently from d18a092 to b31b630 Compare July 14, 2016 01:24

CodaFi force-pushed the argumental branch from b31b630 to dc88e51 Compare July 14, 2016 08:10

CodaFi merged commit 65226de into swiftlang:master Jul 14, 2016

CodaFi deleted the argumental branch July 14, 2016 16:38

CodaFi mentioned this pull request Dec 13, 2016

[Stdlib][ABI] Resolve ABI FIXME #25 #6246

Merged

CodaFi mentioned this pull request Jun 21, 2016

[SR-1119] Process.arguments isn't available when 'main' isn't generated by Swift #43732

Closed

Conversation

CodaFi commented Jun 21, 2016

What's in this pull request?

Resolved bug number: (SR-1119)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr Jun 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr Jun 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jun 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 11, 2016

Uh oh!

CodaFi commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 12, 2016

Uh oh!

CodaFi commented Jul 12, 2016

Uh oh!

CodaFi commented Jul 12, 2016

Uh oh!

CodaFi commented Jul 14, 2016

Uh oh!

CodaFi commented Jul 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gribozavr Jun 21, 2016 •

edited

Loading

gribozavr Jun 21, 2016 •

edited

Loading